[WIP] fixing #2898: no error output if dynamic component with ssr: false crashes #3009

Jabher · 2017-09-28T14:33:57Z

This commit is fixing issue #2898 by adding simple .catch wrapper with window.next.renderError call

Jabher · 2017-10-11T13:09:04Z

Guys, wtf? It is real bug, that is actually making life painful for some people, issue was reported month ago, fix was created 2 weeks ago and you even not commented yet, bug is reproducible and fix is really obvious.

And not even a word was spoken :(

timneutkens · 2017-10-11T19:44:15Z

lib/dynamic.js

@@ -71,6 +71,13 @@ export default function dynamicComponent (p, o) {
          this.state.AsyncComponent = AsyncComponent
        }
      })
+        .catch(
+          this.ssr
+          ? undefined


Won't this silently swallow the error when it happens server side? 🤔

No, it would not make anything worse than it is. Both .catch(cb?) and .then(cb?) are actually a wrappers around .then(successHandler?, errorHandler?). Let me explain it with shell example:

# jabher @ DESKTOP-DTTUH1G in /mnt/c/Users/jabher [1:05:39] $ node > process.on("unhandledRejection", console.warn.bind(null, 'in uncaught rejecton')); void 0 undefined > Promise.reject('test').catch(undefined); void 0 undefined > in uncaught rejecton test Promise { <rejected> 'test', domain: Domain { domain: null, _events: { error: [Function: debugDomainError] }, _eventsCount: 1, _maxListeners: undefined, members: [] } }

Oh, nice! 👌 So returning undefined actually will bubble it up 😄 TIL 👍

timneutkens · 2017-10-11T19:59:13Z

Hey 👋,
First of all thanks for your contribution 🙌 ❤️

Guys, wtf? It is real bug

Lets start by pointing to this: https://www.contributor-covenant.org/version/1/4/code-of-conduct.html

Specifically:

Using welcoming and inclusive language

It is real bug, that is actually making life painful for some people

Sorry to hear that!

issue was reported month ago

As you can see at the moment we have 193 issues, and I'm maintaining Next outside of my day job, meaning all the time I spend outside of work goes towards pushing Next forward.

fix was created 2 weeks ago and you even not commented yet

We've been busy preparing the release of Next v4. Unfortunately I haven't had time to look into all (38) open pull requests. Including yours. I did see your PR though, and assigned Arunoda to review it. I've just sent him a private message asking him to check it out 😄

Again, sorry for your trouble, just trying to explain the context as to why this happened 😄

I've review the PR myself too, just checking if the catch you've added does what we expect it will do 😄 👌

albinekb · 2017-10-11T20:06:13Z

@Jabher if you need this now now now, why not yarn link it? Or point your package.json to your fork? 🤔

Jabher · 2017-10-11T22:00:57Z

@timneutkens

Guys, wtf? It is real bug

Lets start by pointing to this: https://www.contributor-covenant.org/version/1/4/code-of-conduct.html

On one hand - I totally agree about this. On other - everyone answered after a month only when I told that word.
I've seen lots of approaches in working with contributors, but simply ignoring them is really horrible thing. All of "we will look at it later", "sorry, but we don't need it" and "it's not in our code style" are fine, but simply selectively ignoring PRs makes people feel that it is not actually an open-source with community engagement, but public "club of friends".

So, sorry about that, but I was really tired about checking if someone aswered me with at least "not going into sources, make your own fork". Obviously, I wanted to keep on with master and all the community 😅

@Jabher if you need this now now now, why not yarn link it? Or point your package.json to your fork? 🤔

@albinekb next is developing rapidly, I've started a project on 4.0 beta few weeks ago totally sure that it will go in release within few months. And rebasing this commit every second day sounds like not very good option.

albinekb · 2017-10-11T22:19:48Z

Yeah, totally reasonable @Jabher 👍 Well answered. Let's get this one going @timneutkens 🥇 There have just been a lot going on at zeit this month with zeit.co/day, so I can assure you that no one is selectively ignores any PRs :)

Jabher · 2017-10-11T23:33:15Z

Ah, right, zeit conf. Now everything looks extremely reasonable, I know how much power the conf takes, sorry for that 🙌

arunoda · 2017-10-12T00:00:03Z

This is a good check and I think we should ship this ASAP. But before that, I'd like to have a test case for this.
(Here's the place we do it.)

If you have don't time, just tell me. I'll work on it.
Thanks.

timneutkens · 2017-10-12T06:28:34Z

@Jabher 👌

On one hand - I totally agree about this. On other - everyone answered after a month only when I told that word.

A simple ping would have been enough 👌😄 Basically I go through all my notifications every day and respond to everything 👍

timneutkens · 2017-10-12T10:03:17Z

.idea/codeStyleSettings.xml

@@ -0,0 +1,30 @@
+<?xml version="1.0" encoding="UTF-8"?>


Can you remove this file?

Please ignore this for now 😄 Had to run, I usually commit after mostly every significant action. Will squash and cleanup everything when it will be ready!

Great, thaaaaaaaanks! 🎉

Jabher · 2017-10-20T15:03:23Z

Guys, sorry, but I feel I'm stuck - I cannot figure out how to test this, I've tried several different approaches, but none worked.

timneutkens · 2017-11-12T23:08:47Z

@Jabher if you make sure the build succeeds we'll add the test later 👍

fixing #2898

6236b7d

Jabher changed the title ~~fixing #2898~~ fixing #2898: no error output if dynamic component with ssr: false crashes Sep 28, 2017

timneutkens requested a review from arunoda September 28, 2017 18:11

Jabher added 3 commits September 29, 2017 15:01

Merge branch 'master' into master

8ae7d5f

Merge branch 'master' into master

9709b95

Merge branch 'master' into master

b09cff2

timneutkens reviewed Oct 11, 2017

View reviewed changes

Merge branch 'master' into master

6c18334

tests wip

01bc8fa

timneutkens reviewed Oct 12, 2017

View reviewed changes

Merge branch 'master' into master

2fb3b82

timneutkens changed the base branch from master to canary November 23, 2017 13:00

Merge branch 'canary' into master

4d46838

timneutkens added the Status: In Progress label Nov 23, 2017

timneutkens changed the title ~~fixing #2898: no error output if dynamic component with ssr: false crashes~~ [WIP] fixing #2898: no error output if dynamic component with ssr: false crashes Nov 23, 2017

timneutkens mentioned this pull request Feb 26, 2018

Render error when dynamic import throws #3897

Closed

timneutkens closed this Apr 1, 2018

vjpr mentioned this pull request May 7, 2018

Dynamic import() fails in next 5.xx #3775

Closed

1 task

lock bot locked as resolved and limited conversation to collaborators Apr 1, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] fixing #2898: no error output if dynamic component with ssr: false crashes #3009

[WIP] fixing #2898: no error output if dynamic component with ssr: false crashes #3009

Jabher commented Sep 28, 2017 •

edited

Loading

Jabher commented Oct 11, 2017

timneutkens Oct 11, 2017

Jabher Oct 11, 2017

timneutkens Oct 12, 2017

timneutkens commented Oct 11, 2017 •

edited

Loading

albinekb commented Oct 11, 2017

Jabher commented Oct 11, 2017 •

edited

Loading

albinekb commented Oct 11, 2017 •

edited

Loading

Jabher commented Oct 11, 2017

arunoda commented Oct 12, 2017

timneutkens commented Oct 12, 2017 •

edited

Loading

timneutkens Oct 12, 2017

Jabher Oct 12, 2017

timneutkens Oct 12, 2017

Jabher commented Oct 20, 2017

timneutkens commented Nov 12, 2017

[WIP] fixing #2898: no error output if dynamic component with ssr: false crashes #3009

[WIP] fixing #2898: no error output if dynamic component with ssr: false crashes #3009

Conversation

Jabher commented Sep 28, 2017 • edited Loading

Jabher commented Oct 11, 2017

timneutkens Oct 11, 2017

Choose a reason for hiding this comment

Jabher Oct 11, 2017

Choose a reason for hiding this comment

timneutkens Oct 12, 2017

Choose a reason for hiding this comment

timneutkens commented Oct 11, 2017 • edited Loading

albinekb commented Oct 11, 2017

Jabher commented Oct 11, 2017 • edited Loading

albinekb commented Oct 11, 2017 • edited Loading

Jabher commented Oct 11, 2017

arunoda commented Oct 12, 2017

timneutkens commented Oct 12, 2017 • edited Loading

timneutkens Oct 12, 2017

Choose a reason for hiding this comment

Jabher Oct 12, 2017

Choose a reason for hiding this comment

timneutkens Oct 12, 2017

Choose a reason for hiding this comment

Jabher commented Oct 20, 2017

timneutkens commented Nov 12, 2017

Jabher commented Sep 28, 2017 •

edited

Loading

timneutkens commented Oct 11, 2017 •

edited

Loading

Jabher commented Oct 11, 2017 •

edited

Loading

albinekb commented Oct 11, 2017 •

edited

Loading

timneutkens commented Oct 12, 2017 •

edited

Loading